Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expose newtype objects to Mima #4219

Merged
merged 3 commits into from
Jun 11, 2022
Merged

Expose newtype objects to Mima #4219

merged 3 commits into from
Jun 11, 2022

Conversation

satorg
Copy link
Contributor

@satorg satorg commented Jun 2, 2022

The motivation for this PR is discussed here: #4187 (comment)

@satorg satorg added this to the 2.8.0 milestone Jun 2, 2022
@satorg satorg requested a review from armanbilge June 2, 2022 18:38
@satorg satorg self-assigned this Jun 2, 2022
@satorg satorg mentioned this pull request Jun 2, 2022
Closed
@satorg satorg force-pushed the expose-newtype-objects-to-mima branch from bf2194c to c6cc133 Compare June 9, 2022 04:02
@armanbilge armanbilge modified the milestone: 2.8.0 Jun 9, 2022
@satorg satorg force-pushed the expose-newtype-objects-to-mima branch from c6cc133 to 5cd610b Compare June 10, 2022 23:05
@satorg satorg requested a review from armanbilge June 10, 2022 23:07
@satorg
Copy link
Contributor Author

satorg commented Jun 10, 2022

Hmm... Looks like making the NonEmptySetImpt object public makes Mima absolutely unhappy:

[error] Cats core: Failed binary compatibility check against org.typelevel:cats-core_2.13:2.3.0 (e:info.apiURL=https://oss.sonatype.org/service/local/repositories/snapshots/archive/org/typelevel/cats-docs_2.13/2.8-5cd610b-SNAPSHOT/cats-docs_2.13-2.8-5cd610b-SNAPSHOT-javadoc.jar/!/index.html, e:info.versionScheme=early-semver)! Found 1 potential problems
[error]  * static method catsDataEqForNonEmptySet(cats.kernel.Order)cats.kernel.Eq in class cats.data.NonEmptySetImpl does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.data.NonEmptySetImpl.catsDataEqForNonEmptySet")
[error] stack trace is suppressed; run last coreJVM / mimaReportBinaryIssues for the full output
[error] Total time: 25 s, completed Jun 10, 2022 4:35:11 PM
[error] (coreJVM / mimaReportBinaryIssues) Failed binary compatibility check against org.typelevel:cats-core_2.13:2.3.0 (e:info.apiURL=https://oss.sonatype.org/service/local/repositories/snapshots/arc

Whereas the previouls solution (with abstract class) didn't concern Mima at all...

If I said that now I understand just nothing – it wouldn't be too far from the truth...

@armanbilge
Copy link
Member

Right, that's because as I said, we already broke binary compatibility. Now that you've made it public, MiMa is actually able to check bincompat :) this is why I think the is the right strategy forward.

See #4219 (comment) for a patch to fix it.

@satorg
Copy link
Contributor Author

satorg commented Jun 11, 2022

Ah ok, now I got it (finally). Thanks.

@satorg satorg force-pushed the expose-newtype-objects-to-mima branch from 5cd610b to de5357b Compare June 11, 2022 01:32
@satorg
Copy link
Contributor Author

satorg commented Jun 11, 2022

@armanbilge I've updated the PR, take one more look please.

@satorg satorg requested a review from armanbilge June 11, 2022 01:35
armanbilge
armanbilge reviewed Jun 11, 2022
View reviewed changes
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 excellent scaladoc comment, thank you! Just a small typo :)

core/src/main/scala/cats/data/NonEmptyChain.scala Outdated Show resolved Hide resolved
@satorg satorg force-pushed the expose-newtype-objects-to-mima branch from de5357b to 5c2a0c0 Compare June 11, 2022 01:51
armanbilge
armanbilge previously approved these changes Jun 11, 2022
View reviewed changes
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for identifying and investigating this issue so thoroughly!

@satorg satorg requested a review from armanbilge June 11, 2022 01:52
@satorg satorg force-pushed the expose-newtype-objects-to-mima branch from 5c2a0c0 to 96966e4 Compare June 11, 2022 02:52
@satorg satorg force-pushed the expose-newtype-objects-to-mima branch from 96966e4 to d6fb9df Compare June 11, 2022 03:42
@satorg
Copy link
Contributor Author

satorg commented Jun 11, 2022

I think I can merge it now – it shouldn't make things worse somehow.

@satorg satorg merged commit 113da92 into typelevel:main Jun 11, 2022
@satorg satorg deleted the expose-newtype-objects-to-mima branch June 11, 2022 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@armanbilge armanbilge armanbilge approved these changes

Assignees

@satorg satorg

Labels
None yet
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.
2 participants
@satorg @armanbilge